NC versions of PreImages, PreImagesSet, PreImagesElm and PreImagesRepresentative - version 2 - #6409

Codecov Report

❌ Patch coverage is 73.90600% with 322 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.97%. Comparing base (66fd147) to head (ea9c680).
⚠️ Report is 8 commits behind head on master.

@@            Coverage Diff             @@
##           master    #6409      +/-   ##
==========================================
- Coverage   78.97%   78.97%   -0.01%     
==========================================
  Files         684      684              
  Lines      294201   294986     +785     
  Branches     8670     8670              
==========================================
+ Hits       232359   232964     +605     
- Misses      60030    60214     +184     
+ Partials     1812     1808       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@stertooy It's so good to get feedback on this work - I will get on with making the changes you suggest.
The way I have done it most of the time, when the new test fails, PreImagesRepresentative returns fail, but PreImagesSet and PreImagesElm (which expect a list) return []. Is that correct, or should this be fail also for PreImagesElm? With PreImagesSet I can see that Intersection( Range(f),U) should be used.

Now I am really confused. The manual states: "If elms is a subset of the range of the general mapping map then PreImagesSet returns the set of all preimages of elms under map." Sticking to that, PreImagesSet should return fail if U is not a subset of the range. Alternatively, we could ditch PreImagesSetNC, and redefine PreImagesSet to return the preimage of the intersection, which could be [].

Perhaps this is why I returned [] in PreImagesElm when the test failed:
Two existing methods for PreImagesElm in mapphomo.gi call PreImagesRepresentative,
and if that returns fail then [] is returned.
I'll do nothing now until advice is received.

The manual states: "If elms is a subset of the range of the general mapping map then PreImagesSet returns the set of all preimages of elms under map." Sticking to that, PreImagesSet should return fail if U is not a subset of the range.

I think the question is what we want the non-NC functions to do for elements/sets not contained in the range. I see two options:

1. We limit the scope of these functions to elements/sets contained in the range. The only difference between NC and non-NC functions, is that the latter are guaranteed to return fail if the element/set is not contained in the image.

2. The non-NC functions behave like the usual mathematical definition of a preimage. So in this case PreImagesElm( f, x ) would return the empty set [ ] if x is not contained in the image of f, and PreImagesSet( f, U ) would return the same as PreImagesSetNC( f, Intersection( Image( f ), U ) ).

The former option keeps the NC and non-NC versions closer together, with the only difference being a extra check, which is what an NC version usually indicates in GAP. The latter option sticks closer to the standard mathematical definitions. Either option is fine with me (with a preference for option 2), but it should at least be consistent across all the affected functions.

Alternatively, we could ditch PreImagesSetNC, and redefine PreImagesSet to return the preimage of the intersection, which could be [].

I think we should still keep PreImagesNC in this case, since certain functions/methods may have prior knowledge of a set actually being contained in the image, and they can then use the NC version to avoid running a superfluous membership check or intersection calculation.

Perhaps @fingolfin @ThomasBreuer @hulpke want to weigh in, given that they were active in #4809 and #5173?

Thanks for working on this again, @cdwensley. Unfortunately I don't have the bandwidth available to read all here, think about it, and come up with a good stance right now -- we'll get there, but I am afraid not in time for GAP 4.16.0 -- but I am confident it'll get merged eventually, certainly before 4.17.0.

I'll try to get back to this when I have some mental capacity for it

I think part of the reason of NC functions is to actually not have to worry about issues such as membership (which might require an extra test).
I.e. the result of PreImagesSetNC should undefined if the set is not in the image of the map, but if it is returns the same as PreImagesSet.

Advice required about PreImagesSet. If R is the range of map, and if the set U is a subset of R such that the intersection of U and the image of map is V then, mathematically, PreImagesSet(U) is PreImagesSet(V). Is that what should be implemented? At present, fail is returned.

Advice required about PreImagesSet.

I see, PreImagesSet is conceptually different from PreImagesRepresentative.
(Everything is o.k. with the setup for PreImagesRepresentative: The checks are done by the methods for PreImagesRepresentative, then PreImagesRepresentativeNC gets called which does not do any check.)

The documentation of PreImagesSet says that anything may happen if the given set is not a subset of the Range of the map. Here it makes sense to do the same as for PreImagesRepresentative, that is, make a check and signal an error if the set is not a subset of the range.
For the case that the set is a subset of the Range of the map, we could keep the old definition of PreImagesSet, that is, return the set of all preimages of the set under the map, which may be empty.
The point is that with this definition, PreImagesSetNC must deal with the case that the given set is not necessarily a subset of the Image of the map.
Aren't we then back in the situation before this pull request, that is, that some methods don't do this? (I will have to look at the old code.)

For the case that the set is a subset of the Range of the map, we could keep the old definition of PreImagesSet, that is, return the set of all preimages of the set under the map, which may be empty.
The point is that with this definition, PreImagesSetNC must deal with the case that the given set is not necessarily a subset of the Image of the map.
Aren't we then back in the situation before this pull request, that is, that some methods don't do this? (I will have to look at the old code.)

Is it not acceptable to have PreImagesSet( f, U ) do something like

if not IsSubset( Range( f ), U ) then
    Error( "..." );
fi;
V := Intersection( Image( f ), U );
return PreImagesSetNC( f, V );

or perhaps, if U is just a list without additional structure,

if not IsSubset( Range( f ), U ) then
    Error( "..." );
fi;
prei := Set( U, u -> PreImagesRepresentative( f, u ) );
RemoveSet( prei, fail );
return prei;

Something like this means we keep the old definition of PreImagesSet, while the NC versions never have to deal with things outside the image.

Is it not acceptable to have PreImagesSet( f, U ) do something like ...

Proceeding like this means:

• PreImagesSet( map, C ) signals an error if C is not a subset of Range( map ).
• Methods for PreImagesSet( map, C ) may compute inter:= Intersection( C, Image( map ) ) and then call PreImagesSetNC( map, inter ).
• PreImagesSetNC( map, C ) may assume that C is a subset of Image( map ).
• In my own code, when I know that C is a subset of Image( map ), I should use PreImagesSetNC not PreImagesSet, in order to avoid unnecessary computations.
• In my own code, when I know that C is a subset of Range( map ), I should compute Intersection( C, Image( map ) ) myself (perhaps in a better way, depending on the situation), and then use PreImagesSetNC.

I think this is acceptable. We make sue that elements not in the image do not cause trouble, and the NC variant allows one to skip expensive tests where this is possible.

(Delegating from PreImagesSet to PreImagesRepresentative for the elements of the given set is not possible because the map need not be injective.)

The proposal from @ThomasBreuer seems very sensible, and I shall proceed to make the suggested changes.

Many thanks @ThomasBreuer for all these corrections - sorry about the silly error.

Regarding PreImageElm - this only applies to bijective maps, and I see no reason to change it.
Far more unsatisfactory are PreImage and PreImages. These two global functions just delegate to one of three operations, so why are they needed? Furthermore the only difference is between PreImage(map,elm) which calls PreImageElm(map,elm) and PreImages(map,elm) which calls PreImagesElm(map,elm) - the other two cases return the same sets.
Cannot remember why, but an NC version of PreImages has been added, but no PreImageNC. Surely it should be neither or both? I'm inclined to remove PreImagesNC. Advice needed!

Interestingly, PreImage is called around 150 times in the main library, but PreImages not at all.

How many of these should be changed? For example, in ghomperm.gi, line 308,
surely G := PreImage( hom );
should be: G := PreImagesRange( hom );
and (more complicated example with fp-groups) in grpfp.gi, line 948,
hom:=MaximalAbelianQuotient(G);
u:=PreImage(hom,TrivialSubgroup(Range(hom)));
could be:
u:=PreImagesSetNC(hom,TrivialSubgroup(Range(hom)));

Regarding PreImageElm - this only applies to bijective maps, and I see no reason to change it.

The documentation of PreImageElm( map, elm ) says: Anything may happen if elm is not an element of the range of map.
Shouldn't this be changed to a promised error message, for the sake of consistency with the PreImagesSomething operations?
(This need not be part of this pull request.)

Far more unsatisfactory are PreImage and PreImages. These two global functions just delegate to one of three operations, so why are they needed?

The idea was that PreImages( map, obj ) and PreImage( map, obj ) promise an error message if obj is not an element or subset of Range( map ). I do not remember why two functions were introduced.

Thanks for your comments. Away for a few days - will address them when we return.

There was a merge conflict in tst/testinstall/ggmbi.tst‎ (looked like a copy&paste accident) which I just resolved (so you should git pull)

As suggested last week I have removed the recently introduced PreImagesNC from the library, though PreImages remains. In fact there were 12 calls to PreImages in the library which were changed to PreImagesNC and are now back to PreImages (in alglie.gi, grp.gi, grpnice.gi, grpprmcs.gi, mapprep.gi and relation.gi). Entries for PreImages in mapping.gd and mapping.gi have reverted to the versions in master.

Thanks @cdwensley -- I really hope we can finally get this in for GAP 4.17.0

@cdwensley I took the liberty of fixing the merge conflicts with master, so if you want to do more work here, you should make sure to first pull from here.

@ThomasBreuer, @fingolfin : thanks for all these comments. Not sure when I shall be able to address them.
I have pulled as Max suggested - I was finding it very difficult to rebase from master.

I pushed two more commits (with AI assistance, see the Co-authored-by trailers):

9f504d8 addresses the remaining review comments from @ThomasBreuer:

• PreImagesRepresentativeNC (for total non-s.p. general mappings) again returns fail instead of [] when the element has no preimage
• the documentation of the NC variants now says anything may happen if the input is not in the image (rather than: range), matching the checks the non-NC versions perform
• added the missing <Oper> entry for PreImagesElmNC, so ?PreImagesElmNC works
• added the suggested explanatory comment to the composition PreImagesElmNC method

While reviewing I also found and fixed a merge accident in lib/mapping.gi: the last master merge had left duplicated PreImagesSet / PreImagesSetNC method installations, one of which implemented outdated semantics (returning [] for elements outside the image, without a range check). The surviving method implements the agreed behaviour. Plus minor cleanup (a stray semicolon, a missing paren in the docs).

91c2c2b adds tst/testinstall/preimages.tst, which systematically exercises the three branches of the new non-NC wrappers. This should also improve the patch coverage codecov complained about.

Not addressed: the suggestion to turn the "membership check + call NC" pattern into a single generic default method for PreImagesRepresentative (comment from July 7). That would remove a fair amount of per-type boilerplate but is a larger restructuring. So I'd leave it for a follow-up PR... But happy to discuss it some more here.

I have completely lost touch with what is going on here, but believe that there is not much more to do? I hope someone else can take this PR over?

@cdwensley sorry for the chaos, and thank you for shepherding this until here. But I think we are almost done! I'll take this over together with @ThomasBreuer to get it over the finish line soon.

OK history squashed into four commits. Which I have not yet reviewed myself, but others, feel free to do so now. I'll also do it later. I hope we can merge this soon :-)